Pattern block: Ensure consistent editing of overrides in Write Mode #65408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
commented
Sep 17, 2024
Comment on lines
2935
to
3010
| if ( getParentPatternCount( state, clientId ) > 0 ) { | ||
| return getPatternBlockEditingMode( state, clientId ); | ||
| } |
There was a problem hiding this comment.
I haven't tested this with 'zoom-out' mode yet, so it might need some adjustment.
There was a problem hiding this comment.
Screen.Capture.on.2024-09-19.at.09-10-09.mp4
I tested it. The only quirk is that the overridable fields highlight when the block is clicked.
There was a problem hiding this comment.
The code for flashing blocks is part of the props passed to useFlashEditableBlocks hook in packages/block-editor/src/components/block-list/use-block-props/index.js
There was a problem hiding this comment.
This one is a feature to show which parts of the pattern are editable, I think @noisysocks worked on it.
There was a problem hiding this comment.
It would actually be a good feature if we could edit those fields in zoom out view. In the future we might want to allow that.
There was a problem hiding this comment.
Ah, I see, I didn't realize they weren't editable. I've pushed a commit that adjusts that.
There was a problem hiding this comment.
This is our first "fork" situation. When #65702 is merged we will want Gutenberg trunk to flash blocks but WP 6.7 not to flash blocks (because in 6.7 you can't edit sub content) 😬
There was a problem hiding this comment.
🤔 Maybe we can utilize IS_GUTENBERG_PLUGIN for the bit that's different.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
changed the title
talldan
commented
Sep 17, 2024
| * | ||
| * @return {number} The number of parent pattern blocks. | ||
| */ | ||
| export function getParentPatternCount( state, clientId ) { |
There was a problem hiding this comment.
Might be an idea to make this one memoized, and also check that the memoization of selectors that use getBlockEditingMode is still correct 🤔
|
Size Change: -83 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
talldan
commented
Sep 17, 2024
| @@ -3043,7 +3043,7 @@ describe( 'selectors', () => { | |||
| byClientId: new Map( | |||
| Object.entries( { | |||
| block1: { name: 'core/test-block-ancestor' }, | |||
| block2: { name: 'core/block' }, | |||
| block2: { name: 'core/group' }, | |||
There was a problem hiding this comment.
This test was failing because it tries to insert a block within a child of a core/block block. This wouldn't be possible by a user in trunk (as it's disallowed by the block's 'edit') and it also failed in unit tests in this PR now that the block editing mode is enforced at selector level.
I've switched it to a group instead.
youknowriad
force-pushed
the
update/content-only-select-mode
branch
from
0475922
to
cf9cedf
Compare
youknowriad
requested review from
juanmaguitar,
fabiankaegy and
ndiego
as code owners
talldan
force-pushed
the
fix/pattern-blocks-in-select-mode
branch
from
7b4d6de
to
b078708
Compare
|
Flaky tests detected in 388cae1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10939232475
|
|
This test very well for me. I also noticed what @getdave mentioned that when zoomed out to compose patterns the overrides highlight on click - it'd be great to remove that then but this is out of the scope of this - this fixes editing overrides in the new edit mode. Great work, will you merge this @talldan ? |
talldan
force-pushed
the
fix/pattern-blocks-in-select-mode
branch
from
b078708
to
388cae1
Compare
That should be fixed by the latest commit. @youknowriad Are you happy for this to merged into your branch? I can also try basing it on trunk, though the things it fixes are more relevant to that branch. |
|
@talldan Let's keep it separate. I believe my PR is an approval away from being merged. I think it's better for the history to keep these two separate. We can merge this right after. |
talldan
force-pushed
the
fix/pattern-blocks-in-select-mode
branch
from
388cae1
to
b42257b
Compare
|
I've rebased this PR. I'm not sure how relevant it still is, I've fallen a bit behind all the work that's been happening lately. |
youknowriad
reviewed
Oct 1, 2024
| * | ||
| * @return {string} The block editing mode. | ||
| */ | ||
| export function getPatternBlockEditingMode( state, clientId ) { |
There was a problem hiding this comment.
Why is this a separate selector not part of the more generic getBlockEditingMode?
There was a problem hiding this comment.
I thought it might be more readable. I don't have a strong preference though, I've pushed a commit that makes it one selector.
There was a problem hiding this comment.
Ah, making it separate for clarity like an internal function is fine but the problem with it exposed as a separate selector is the fact that the "framework" when it needs to know the block's editing mode has to switch between two different selectors.
Fix broken `getEnabledBlockParents` selector unit tests Update test Fix flash of content when in zoom out mode
talldan
force-pushed
the
fix/pattern-blocks-in-select-mode
branch
from
b42257b
to
ec59ed1
Compare
youknowriad
reviewed
Oct 2, 2024
| ); | ||
|
|
||
| // Sync the editing mode of the pattern block with the inner blocks. | ||
| useEffect( () => { | ||
| setBlockEditMode( |
There was a problem hiding this comment.
Nice to see this gone :)
talldan
changed the title
draganescu
approved these changes
Oct 7, 2024
There was a problem hiding this comment.
Ha! This PR now fixes a different problem: initially the problem was that editable parts of synced patterns were not editable in write mode. However that is fixed now in trunk.
But there is another bug in trunk that this fixes: in trunk in write mode you can edit uneditable parts of a synced pattern. With this applied that is fixed.
|
Is it possible this PR introduced a regression to typing metrics:
|
@tyxla Thanks for catching. Yes, from what I can tell |
|
Reverted in #65953 |
|
I wonder if we could/should look at why |
|
It seems understandable that In other words, add a "computed" |
6 tasks
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ordPress#65408) * Move pattern block block editing state to store selector Fix broken `getEnabledBlockParents` selector unit tests Update test Fix flash of content when in zoom out mode * Combine into one selector * Add unit tests * Add zoom out pattern block editing mode test ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org>
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…e Mode (WordPress#65408)" (WordPress#65953) This reverts commit ace9735. ---- Co-authored-by: tyxla <tyxla@git.wordpress.org>
This was referenced Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #65204 and merges into that PR.
What?
The pattern (core/block) block uses an effect to set
blockEditingModefor its inner blocks to the correct thing.This PR moves that calculation to a selector so that it plays better with other modes.
Why?
The
getBlockEditingModehas some internal logic for different editing modes (likezoom-outand the content only behavior in #65204) that can override the actual block editing state for a pattern's inner blocks.How?
Moving the state to the selector it should be possible to make the modes work together with pattern overrides in the expected way.
Testing Instructions
There's one use case that I can see that doesn't seem to work, but there might be others
Screenshots or screencast
Before
An pattern that's a child of a 'section' group has overrides that can't be edited in Edit Mode (even though they're 'content')
After
The pattern overrides can be selected and edited in Edit Mode
